[Clang][HLSL][GVN] Prevent phi codgen of isTokenLike types #162363
Conversation
fixes llvm#161754 When the GVN pass is PerformLoadPRE we need to check if it might be doing this load on a token like type. This is because if we don't GVN will use the SSAUpdater to insert a phi node to reduce duplicate resource.getpointer calls. Because in an earlier commit: llvm@01c0a84 we made the verifier error with `PHI nodes cannot have token type!` This test case will fail today if we try to perform this load optimization https://godbolt.org/z/xM69fY8zM
|
@llvm/pr-subscribers-llvm-transforms Author: Farzon Lotfi (farzonl) Changesfixes #161754 When the GVN pass is PerformLoadPRE we need to check if it might be doing this load on a token like type. This is because if we don't GVN will use the SSAUpdater to insert a phi node to reduce duplicate resource.getpointer calls. Because in an earlier commit: 01c0a84 we made the verifier error with This test case will fail today if we try to perform this load optimization This will impact clang aswell because Full diff: https://github.com/llvm/llvm-project/pull/162363.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index b9b5b5823d780..0fa99eb375a47 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1659,6 +1659,9 @@ bool GVNPass::PerformLoadPRE(LoadInst *Load, AvailValInBlkVect &ValuesPerBlock,
// that we only have to insert *one* load (which means we're basically moving
// the load, not inserting a new one).
+ if (Load->getType()->isTokenLikeTy())
+ return false;
+
SmallPtrSet<BasicBlock *, 4> Blockers(llvm::from_range, UnavailableBlocks);
// Let's find the first basic block with more than one predecessor. Walk
diff --git a/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll b/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll
new file mode 100644
index 0000000000000..7fc56e8b5d8f1
--- /dev/null
+++ b/llvm/test/Transforms/GVN/PRE/no-pre-load-for-token-like.ll
@@ -0,0 +1,34 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -passes=gvn %s | FileCheck %s
+
+; NOTE: A test to confirm GVN doesn't introduce phis for pre loads of token
+; NOTE: like types. This implies the CHECKS should exactly match the IR.
+
+define ptr @main() {
+; CHECK-LABEL: define ptr @main() {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: br i1 false, label %[[ENTRY_IF_END_I_CRIT_EDGE:.*]], label %[[IF_THEN_I:.*]]
+; CHECK: [[ENTRY_IF_END_I_CRIT_EDGE]]:
+; CHECK-NEXT: br label %[[IF_END_I:.*]]
+; CHECK: [[IF_THEN_I]]:
+; CHECK-NEXT: [[TMP0:%.*]] = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+; CHECK-NEXT: [[TMP1:%.*]] = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) [[TMP0]], i32 0)
+; CHECK-NEXT: br label %[[IF_END_I]]
+; CHECK: [[IF_END_I]]:
+; CHECK-NEXT: [[TMP2:%.*]] = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+; CHECK-NEXT: [[TMP3:%.*]] = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) [[TMP2]], i32 0)
+; CHECK-NEXT: ret ptr [[TMP3]]
+;
+entry:
+ br i1 false, label %if.end.i, label %if.then.i
+
+if.then.i: ; preds = %entry
+ %0 = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+ %1 = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) %0, i32 0)
+ br label %if.end.i
+
+if.end.i: ; preds = %if.then.i, %entry
+ %2 = load target("dx.RawBuffer", half, 1, 0), ptr null, align 4
+ %3 = tail call ptr @llvm.dx.resource.getpointer.p0.tdx.RawBuffer_f16_1_0t(target("dx.RawBuffer", half, 1, 0) %2, i32 0)
+ ret ptr %3
+}
|
There was a problem hiding this comment.
The change looks good to me and it successfully compiles the reduced shader I sent you over Teams. However, the original DML shaders are still having the same issue - hitting the same assertion. So this PR does not completely fix the issue.
I will send you some more shaders to test and debug.
I have a fix to move the check up two functions, but I'm still trying to work out a minified reproductions. llvm-reduce is getting stuck in an infinite loop. diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 0fa99eb375a4..995fd1458598 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -2003,6 +2006,9 @@ bool GVNPass::processNonLocalLoad(LoadInst *Load) {
Load->getParent()->getParent()->hasFnAttribute(
Attribute::SanitizeHWAddress))
return false;
+
+ if (Load->getType()->isTokenLikeTy())
+ return false;
// Step 1: Find the non-local dependencies of the load.
LoadDepVect Deps; |
…ssNonLocalLoad cases
farzonl
added
llvm:GVN
farzonl
changed the title
farzonl
changed the title
fixes #161754
When the GVN pass is PerformLoadPRE we need to check if it might be doing this load on a token like type. This is because if we don't GVN will use the SSAUpdater to insert a phi node to reduce duplicate resource.getpointer calls.
Because in an earlier commit: 01c0a84 we made the verifier error with
PHI nodes cannot have token type!This test case will fail today if we try to perform this load optimization
https://godbolt.org/z/xM69fY8zM
This will impact clang aswell because
isTokenLikeTyalso checks forisTokenTyClang is likely also failing validation with token types but just doesn't have a test case because the validator would error if it were in a phi node.